Skip to content

Validate configured -alertmanager.web.external-url and fail if ends with / #4081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 16, 2021
Merged

Validate configured -alertmanager.web.external-url and fail if ends with / #4081

merged 3 commits into from
Apr 16, 2021

Conversation

@pracucci
Copy link
Contributor

What this PR does:
Earlier this week I was testing the alertmanager in the local setup and I've learned the hard way that ruler->alertmanger notification doesn't work in the local dev setup and that if you configure -alertmanager.web.external-url with an ending / the routing doesn't work as expected.

This PR fixes the local dev env and adds a validation rule on -alertmanager.web.external-url.

Which issue(s) this PR fixes:
N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pracucci
Fixed linter error
db81b13 
Signed-off-by: Marco Pracucci <[email protected]>
@pstibrany
Copy link
Contributor

I've learned the hard way that ruler->alertmanger notification doesn't work in the local dev setup and that if you configure -alertmanager.web.external-url with an ending /

Instead of erroring out, can we fix the problem with /? Either by removing it from the path, or using path.Join when appending. Why does having / in external-url fail in the first place?

@pracucci
Copy link
Contributor Author

Why does having / in external-url fail in the first place?

I don't have time to investigate it (I consider it low priority). I see two options: accept the validation as a signal of a misconfiguration or I can roll it back and we just keep the fix for the dev env.

@pstibrany
Copy link
Contributor

I see two options: accept the validation as a signal of a misconfiguration or I can roll it back and we just keep the fix for the dev env.

Validation is better than rollback. Approved. We can improve it when there is more time.

@pracucci pracucci merged commit 8851245 into cortexproject:master Apr 16, 2021
@pracucci pracucci deleted the fix-alertmanager-local-dev-and-config-validation branch April 16, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@pstibrany pstibrany pstibrany approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pracucci @pstibrany